Auto select when only one NABSL exists#193
Conversation
NicholasYancey
added
the
enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCreateOptions.Complete now defers config default lookup until after the Kubernetes client and namespace are available; if StorageLocation remains unset, it lists NonAdminBackupStorageLocation in the current namespace, filters to NonAdminPhaseCreated, and auto-selects when exactly one usable NABSL exists. ChangesStorage Location Resolution
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/non-admin/backup/create.go`:
- Around line 176-189: The auto-select message is printed in Complete
(fmt.Printf in the block that sets o.StorageLocation using nabslList) which
breaks -o yaml/json output; change CreateOptions to add a boolean flag (e.g.,
storageLocationAutoSelected bool) set to true inside Complete when you
auto-select o.StorageLocation from nabslList, remove the fmt.Printf from
Complete, and then in Run—after the PrintWithFormat early return and alongside
the existing storageLocationFromConfig handling—emit the user-facing message
about auto-selection if storageLocationAutoSelected is true so the message is
suppressed for formatted output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d11b8f22-d7a8-491f-934c-6bdf1df95fed
📒 Files selected for processing (1)
cmd/non-admin/backup/create.go
NicholasYancey
force-pushed
the
auto-select-nabsl
branch
2 times, most recently
from
9f48cd2 to
29f1a7d
Compare
kaovilai
left a comment
kaovilai
left a comment
There was a problem hiding this comment.
Should we add a warning that, if the user creates another non-admin BackupStorageLocation in the future, the backups may not go to the same location?
NicholasYancey
force-pushed
the
auto-select-nabsl
branch
from
29f1a7d to
93f6072
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates non-admin backup creation so the CLI can resolve a storage location automatically when the user does not pass --storage-location.
Changes:
- Adds tracking for auto-selected storage locations.
- Moves default NABSL lookup after namespace/client setup.
- Lists NABSLs in the current namespace and auto-selects when exactly one exists.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NicholasYancey
force-pushed
the
auto-select-nabsl
branch
from
93f6072 to
28bde6a
Compare
kaovilai
left a comment
kaovilai
left a comment
There was a problem hiding this comment.
will see if others wants this
|
@weshayutin @mpryc thoughts? |
|
If there is only one approved NABSL I think tthis is a great idea. Once there is more than one NABSL ( approved / not approved ) the user should be prompted to choose. I would need to get setup for a more indepth test |
@coderabbitai create issue to track |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh issue create \
--repo migtools/oadp-cli \
--title "Prompt user to choose NABSL when more than one exists" \
--body "## Summary
When a user has more than one NonAdminBackupStorageLocation (NABSL) in their namespace (approved or not approved), the CLI should prompt the user to choose which storage location to use for nonadmin backup creation, rather than failing or requiring manual flag input.
## Background
PR `#193` (https://github.com/migtools/oadp-cli/pull/193) introduced auto-selection of the storage location when exactly **one** NABSL exists in the namespace. This follow-up issue tracks the complementary behavior: interactive prompting when **multiple** NABSLs exist.
## Desired Behavior
- If exactly one NABSL exists → auto-select it (already implemented in `#193`)
- If more than one NABSL exists (regardless of approved/not approved status) → prompt the user interactively to pick one before proceeding with backup creation
## References
- PR `#193`: https://github.com/migtools/oadp-cli/pull/193
- Requested by `@kaovilai`, noted by `@weshayutin`" \
--assignee kaovilaiLength of output: 1197
Prompt user to choose NABSL when more than one exists The issue captures the desired behavior:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath, kaovilai, NicholasYancey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/cherry-pick oadp-1.6 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@kaovilai: new pull request created: #197 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
consistent with pr migtools#193, only uses usable nabsl Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Why the changes were made
there is only one valid option.
How to test the changes made
oc oadp nonadmin bsl getoc oadp nonadmin backup create test-backupExpected output:
Auto-selected storage location: (only NABSL in namespace)
NonAdminBackup request "test-backup" submitted successfully.
Summary by CodeRabbit
New Features
Bug Fixes